Skip to content

[RISCV] Fix QC.E.LI -> C.LI with Bare Symbol Compression #146763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lenary
Copy link
Member

@lenary lenary commented Jul 2, 2025

There's a long comment explaining this approach in RISCVInstrInfoXqci.td

This is presented as an alternative to #146184.

There's a long comment explaining this approach in RISCVInstrInfoXqci.td

This is presented as an alternative to llvm#146184.
@MaskRay
Copy link
Member

MaskRay commented Jul 10, 2025

#139273 introduced a CompressPat to convert qe.e.li to c.li.

let isCompressOnly = true in
def : CompressPat<(QC_E_LI GPRNoX0:$rd, simm6:$imm),
                  (C_LI GPRNoX0:$rd, simm6:$imm)>;

There is a crash when assembling qc.e.li a0, undef.

Immediate thought: Is simm6 appropriate in the CompressPat? Shall we allow immediates only?

But that would rule out compression for qc.e.li x10, foo; foo = 8, which you probably want.
(It assembles to 051f 0000 0008 qc.e.li a0, 0x80000 with this PR, which suggests a bug.)

Anyhow, let's continue.

  • RISCVAsmParser::emitToStreamer calls RISCVRVC::compress, which calls RISCVValidateMCOperandForCompress to simm6.
  • The verifier passes because MCOp.isBareSymbolRef(); returns true.
  case 3: {
    // simm6
    int64_t Imm;
    if (MCOp.evaluateAsConstantImm(Imm))
      return isInt<6>(Imm);
    return MCOp.isBareSymbolRef();
  }
  • RISCVMCCodeEmitter::encodeInstruction calls getBinaryCodeForInstr for the compressed c.li, which failed because there wasn't a defined fixup.
    This PR adds a fixup, which could be used to accept c.li a0, foo; foo = 9 (currently rejected by GAS, but with a fixup this could be made working)

  • Introducing a fixup for c.li seems like a solid choice.
  • You might want to address the qc.e.li x10, foo; foo = 8 bug.
  • I recommend that RISC-V avoids adding more hacks to the generic interface, such as MCInst MCOp.isBareSymbolRef(). [RISCV] Align MCOperandPredicates with AsmParser #146184 actually eliminates some, which I like. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants